chore(*): cleanup msie handling; add support comments #15407

mgol · 2016-11-17T13:54:17Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Chore

What is the current behavior? (You can also link to an open issue here)
N/A

What is the new behavior (if this is a feature change)?
N/A

Does this PR introduce a breaking change?
No

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format

Other information:

The conditions checking the msie variable value have been simplified.
There is e.g. no point to check if msie <= 11 since there IE 12 won't ever
exist.
Edge UA-sniffing has been added to tests (only!) where appropriate
Support comments for IE/Edge have been added.

1. The conditions checking the msie variable value have been simplified. There is e.g. no point to check if `msie <= 11` since there IE 12 won't ever exist. 2. Edge UA-sniffing has been added to tests (only!) where appropriate 3. Support comments for IE/Edge have been added.

Narretz · 2016-11-17T17:13:26Z

test/ng/directive/booleanAttrsSpec.js

-  if (msie) {
-    // IE11/10/Edge fail when setting a href to a URL containing a % that isn't a valid escape sequence
+  // Support: IE 9-11 only, Edge 12-14+
+  if (msie || /\bEdge\/[\d\.]+\b/) {


Wouldn't it make more sense to put Edge in a variable, same as msie? This will also be useful once we finally test on Edge.

Doesn't this need a .test(...)? Right now it will be true 100% of the time...

@jbedard Nice catch. This means, though, that we can just remove the if and run the test everywhere.

@Narretz A variable would make it easier to reuse and we should avoid UA-sniffing (it's more acceptable in tests but still); it's not robust, it can catch more browsers than intended and cause compatibility concerns. This is actually a good example as the if was unnecessary, I'll remove it.

IE sniffing is different as:

Browsers are trying to pretend they have nothing to do with IE; differently to e.g. Chrome which is imitated by lots of browsers.

We don't do it via the lying user agent string but the non-standard document.documentMode that other browsers will never implement.

There are lots of differences between IE & modern browsers and the gap will only get larger. Edge gets updated and doesn't lag so much so it won't apply to it.

IE sniffing is, therefore, one of the few safe browser sniffing available.

Ah, no, this particular condition was necessary, the tests are now failing. Fixing...

Narretz · 2016-11-17T17:14:14Z

test/ng/rootScopeSpec.js

-      if (msie < 11) return;
-      // eslint-disable-next-line no-proto
-      expect($rootScope.__proto__).toBe($rootScope.constructor.prototype);
+      expect(Object.getPrototypeOf($rootScope)).toBe($rootScope.constructor.prototype);


Why is the msie < 11 check gone?

It was there only because of the Annex B (i.e. compatibility "don't use" stuff) __proto__ that wasn't present in IE <11. The standard ES5+ way to get the prototype is Object.getPrototypeOf(object) which works in IE9+.

mgol · 2016-11-17T23:06:08Z

@Narretz PR updated; PTAL.

gkalpak · 2016-11-18T12:13:34Z

src/Angular.js

@@ -1279,6 +1280,7 @@ function fromJson(json) {

 var ALL_COLONS = /:/g;
 function timezoneToOffset(timezone, fallback) {
+  // Support: IE 9-11 only, Edge 13-14+


What is the difference between Edge 13-14+ and Edge 13+?

In the former we've checked that it, indeed, is broken in both v13 & v14. In the latter we've just checked v13 and the v14 state is unknown.

gkalpak · 2016-11-18T12:45:15Z

test/ng/compileSpec.js

@@ -11154,6 +11162,8 @@ describe('$compile', function() {
    }));
  });

+  // Support: IE 9-10 only
+  // IE <=11 don't support srcdoc


<= --> <

gkalpak · 2016-11-18T12:47:19Z

test/ng/compileSpec.js

@@ -11154,6 +11162,8 @@ describe('$compile', function() {
    }));
  });

+  // Support: IE 9-10 only
+  // IE <=11 don't support srcdoc
  if (!msie || msie >= 11) {


This could be !msie || msie === 11 (for consistency).

gkalpak · 2016-11-18T12:54:12Z

test/ng/rootScopeSpec.js

@@ -125,6 +123,8 @@ describe('Scope', function() {
      function Listener() {
        expect(this).toBeUndefined();
      }
+      // Support: IE 9 only
+      // IE 9 doesn't support strict mode so its `this` will always be defined.
      if (msie < 10) return;


This could be msie === 9.

gkalpak · 2016-11-18T12:55:49Z

test/ng/locationSpec.js

-      if (msie) return;
+    afterEach(function() {
+      // Support: non-Windows browsers
+      if (msie || /\bEdge\/[\d\.]+\b/.test(window.navigator.userAgent)) return;


It would be nice to have this as a reusable flag in tests.

I guess it doesn't hurt when it's used just in tests... (I would definitely not want sth like that in source). But would you like to have it in tests globally or just in locationSpec? If globally then we should define similar variables for other browsers.

I'm a little afraid of making it too easy to use UA-sniffing, though.

No, it doesn't hurt. It is just about the mental overhead of "deciphering" /\bEdge\/[\d\.]+\b/.test(window.navigator.userAgent) vs isEdge.

I'm a little afraid of making it too easy to use UA-sniffing, though.

Fair enough 😃

gkalpak

LGTM

mgol · 2016-11-21T12:58:56Z

@Narretz PTAL, your review is blocking this. :)

1. The conditions checking the msie variable value have been simplified. There is e.g. no point to check if `msie <= 11` since there IE 12 won't ever exist. 2. Edge UA-sniffing has been added to tests (only!) where appropriate 3. Support comments for IE/Edge have been added. Closes angular#15407

mgol added the type: chore label Nov 17, 2016

mgol self-assigned this Nov 17, 2016

googlebot added the cla: yes label Nov 17, 2016

Narretz suggested changes Nov 17, 2016

View reviewed changes

fixup! chore(*): cleanup msie handling; add support comments

d297d8e

gkalpak reviewed Nov 18, 2016

View reviewed changes

fixup! chore(*): cleanup msie handling; add support comments

fa8eb15

gkalpak approved these changes Nov 18, 2016

View reviewed changes

gkalpak added this to the Backlog milestone Nov 18, 2016

Narretz approved these changes Nov 22, 2016

View reviewed changes

petebacondarwin closed this in cc92da0 Nov 23, 2016

mgol deleted the msie-cleanup branch November 28, 2016 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(*): cleanup msie handling; add support comments #15407

chore(*): cleanup msie handling; add support comments #15407

mgol commented Nov 17, 2016

Narretz Nov 17, 2016 •

edited

Loading

jbedard Nov 17, 2016

mgol Nov 17, 2016

mgol Nov 17, 2016

Narretz Nov 17, 2016

mgol Nov 17, 2016

mgol commented Nov 17, 2016

gkalpak Nov 18, 2016

mgol Nov 18, 2016

gkalpak Nov 18, 2016

gkalpak Nov 18, 2016

gkalpak Nov 18, 2016

gkalpak Nov 18, 2016

mgol Nov 18, 2016

gkalpak Nov 18, 2016

gkalpak left a comment

mgol commented Nov 21, 2016

chore(*): cleanup msie handling; add support comments #15407

chore(*): cleanup msie handling; add support comments #15407

Conversation

mgol commented Nov 17, 2016

Narretz Nov 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgol commented Nov 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak left a comment

Choose a reason for hiding this comment

mgol commented Nov 21, 2016

Narretz Nov 17, 2016 •

edited

Loading